Repository navigation
Retry retryable ack failures instead of abandoning the work item (redux) - #72
Open
Thom Chiovoloni (thomcc-work) wants to merge 6 commits into
Open
Thom Chiovoloni (thomcc-work) wants to merge 6 commits into
Thom Chiovoloni (thomcc-work) wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the documented retry policy and has comprehensive deterministic regression coverage.
0 open findings
What changed in this PR
Adds bounded retries for transient activity acknowledgement failures, reducing unnecessary activity re-execution while preserving at-least-once semantics.
Changes:
- Retries acknowledgements up to five times with exponential backoff.
- Adds fault-injection coverage for success, failure, cancellation, exhaustion, and uncertain commits.
- Documents acknowledgement retry and atomicity requirements.
| File | Description |
|---|---|
src/runtime/dispatchers/worker.rs |
Implements acknowledgement retries. |
tests/common/fault_injection.rs |
Adds retryable failure injection and call tracking. |
tests/worker_reliability_test.rs |
Adds acknowledgement retry scenarios. |
tests/session_e2e_tests.rs |
Aligns assertion with at-least-once execution. |
docs/provider-implementation-guide.md |
Documents retry and commit semantics. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Thom Chiovoloni (thomcc-work)
marked this pull request as ready for review
October 13, 2026 16:53
The worker dispatcher abandoned an activity work item on any ack_work_item error, which redelivers the item and re-runs the handler. The dispatcher poll loop already branches on ProviderError::is_retryable() and backs off before retrying fetch_work_item; the ack path ignored the classification entirely. Add ack_work_item_with_retry: up to 5 attempts with exponential backoff (100ms..1s, ~1.5s total) for retryable errors, then fall back to the existing abandon path. Permanent errors still abandon immediately, so poison handling and the existing failure tests are unaffected. This is reachable from any provider that reports transient failures. It is easiest to observe with SqliteProvider::new_in_memory, which uses sqlite::memory:?cache=shared. Shared-cache mode uses table-level locks and returns SQLITE_LOCKED on a read-to-write upgrade collision without invoking the busy handler, so PRAGMA busy_timeout does not cover it. Also relax test_multi_worker_heterogeneous_config, which asserted `a + b == 3`. Activities are documented as at-least-once, so any redelivery makes equality wrong; assert `a + b >= 3` instead. Fixes #41 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Softened the uncertain-ack log so it no longer promises redelivery. - Added duroxide_activity_ack_uncertain_total, counted once per uncertain outcome without inflating failure counters. - Updated metric documentation and added an Unreleased entry in CHANGELOG.md.
- Restored MetricsSnapshot’s original fields and added MetricsProvider::activity_ack_uncertain(). Updated callers and documentation. A new exhaustive struct-literal test reproduced E0063 before the fix and now passes. - Added the missing telemetry specification entry and assertions that both confirmed-failure paths leave uncertainty at zero.
Thom Chiovoloni (thomcc-work)
force-pushed
the
pinodeca/retry-retryable-ack-failures
branch
from
November 2, 2026 18:44
9a6a321 to
71032ce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #41. Taking over for #42.
The worker dispatcher previously abandoned a work item on every acknowledgement error, including retryable provider failures. Abandonment discards the completed activity's result and redelivers the item, potentially running the handler again. That is permitted by at-least-once execution, but is avoidable for transient acknowledgement failures.
This change retries acknowledgements in place for activity success, application errors, and cancellation. There are at most five total attempts, with waits of 100, 200, 400, and 800 milliseconds between attempts. The waits total 1.5 seconds of cumulative backoff, excluding provider-call duration; this is not a wall-clock deadline. Permanent errors and exhausted retries use the existing abandonment path.
The rebased worker retains lock renewal while a normal completion's acknowledgement is pending. Activities still require at-least-once-safe behavior: crashes, lost locks, and exhausted retries can cause re-execution. Poison-message handling, provider APIs, persisted schemas, and SQLite pool configuration are unchanged.
The tests now check exact acknowledgement/abandonment sequences, lock tokens, and completion payloads:
These fault-injection tests use file-backed SQLite so shared-cache deadlocks do not interfere with their exact call-count assertions. The existing in-memory, multi-worker session test remains a separate regression check.
Checklist
cargo nt)cargo test --doc --all-features)docs/if introducing a new areadocs/README.mdLinks / Design notes
SQLite diagnosis
The original issue and PR description attributed the shared-cache failure to SQLx not using SQLite unlock notifications. That explanation is incorrect for SQLx 0.8.6, used in this checkout: its statement execution loop already calls
sqlite3_unlock_notify, and the dependency enables unlock-notification support.SQLite emits the reported
database is deadlockederror when unlock-notification registration detects a wait cycle. Waiting longer cannot resolve that cycle. SQLite documents rollback and later retry of the transaction as the recovery mechanism. The failed acknowledgement transaction is rolled back,and the next provider call opens a fresh transaction.
This supports retrying the complete acknowledgement operation rather than rerunning the activity. The same policy also applies to other providers' retryable failures. A connection failure may leave a commit outcome unknown; atomic deletion plus lock-token validation prevents a repeated acknowledgement from enqueueing the completion again if the first call already committed.
Historical measurements
With the strict, exactly-once assertion retained as a measurement probe, Pino reported 3 failures in 14 runs before the runtime fix and 0 in 20 after it. The latter runs recorded 19 acknowledgement retries and 741
SQLITE_LOCKEDevents. Those measurements were not repeated during the takeover; the current validation uses deterministic fault injection and the regression tests listed above.